Initial support for ext_oneapi_composite_device.#12178
Initial support for ext_oneapi_composite_device.#12178steffenlarsen merged 33 commits intointel:syclfrom
Conversation
|
|
||
| set(LEVEL_ZERO_LOADER_REPO "https://github.com/oneapi-src/level-zero.git") | ||
| set(LEVEL_ZERO_LOADER_TAG v1.11.0) | ||
| set(LEVEL_ZERO_LOADER_TAG v1.15.1) |
There was a problem hiding this comment.
From what I tested during development of this patch, I'd say this is not used. It seems that we're now using the equivalent in URT repo.
There was a problem hiding this comment.
This change was made to make testing possible during development. It should be updated when the corresponding PR (oneapi-src/unified-runtime#1192) in URT is merged.
sycl/source/detail/device_impl.cpp
Outdated
There was a problem hiding this comment.
Why do we have to hardcode that?
There was a problem hiding this comment.
Well, we don't have to, but we know this extension only works for L0 backend, so we can save the call to PI just by checking this.
There was a problem hiding this comment.
I don't like this, but I can live with that. Please add a comment that this is just a performance optimization though. Same below.
sycl/source/platform.cpp
Outdated
There was a problem hiding this comment.
Similar to a question earlier, why do we have to know that in the SYCL RT?
There was a problem hiding this comment.
In this case, the comment is just to point out that only GPU architectures can be a composite device, and that's why we only get GPU devices. The Intel part is not really important, but it is informative. I'm open to removing it, if you think it's confusing or does not help.
There was a problem hiding this comment.
Same as above - I don't like it but I can live with it.
Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@aelovikov-intel I think I addressed all your concerns. |
There was a problem hiding this comment.
This will have to go away before this can be formally approved.
There was a problem hiding this comment.
Yes, definitely, it is just to make sure the CI uses the correct UR version.
There was a problem hiding this comment.
nit: we have an std::hash<device> specialization, so I'd imagine std::set could work here. Feel free to ignore.
There was a problem hiding this comment.
That can be an std::copy_if(Composites.begin(), Composites.end(), std::back_inserter{Result}, [](...) { /* predicate */ });
sycl/source/detail/device_impl.cpp
Outdated
There was a problem hiding this comment.
I don't like this, but I can live with that. Please add a comment that this is just a performance optimization though. Same below.
| sycl::device, ext::oneapi::experimental::info::device::composite_device> { | ||
| static sycl::device get(const DeviceImplPtr &Dev) { | ||
| if (Dev->getBackend() != backend::ext_oneapi_level_zero) | ||
| return {}; |
There was a problem hiding this comment.
Why not throwing an exception?
There was a problem hiding this comment.
From the extension specification:
The APIs may be called even when using other backends, but they will return an empty list of composite devices.
There was a problem hiding this comment.
| if (!IsL0 || !IsCombined) | |
| assert(CompositeDevs.empty()); | |
| assert(CompositeDevs.empty() || (IsL0 && IsCombined)); |
| if (std::find(CombinedCompositeDevs.begin(), | ||
| CombinedCompositeDevs.end(), | ||
| D) == CombinedCompositeDevs.end()) | ||
| CombinedCompositeDevs.push_back(D); |
There was a problem hiding this comment.
Why do we need this check? Devices across platforms can't be the same and I would not expect to have duplicate devices in P.ext_oneapi_get_composite_devices().
There was a problem hiding this comment.
We're just checking that the following statement from extension spec holds for our implementation:
The free function
get_composite_devicesreturns all of the composite devices across all platforms. The member functionplatform::ext_oneapi_get_composite_devicesreturns the composite devices within the given platform.
Particularly, we are checking that the free function returns the composite devices across all platforms, and we are not missing any due to a bug.
There was a problem hiding this comment.
Why do we have a guarantee that the order must be the same?
There was a problem hiding this comment.
Good catch! We don't in this case. We do have to guarantee that several calls to the same function must have the same order, but this is not the case.
There was a problem hiding this comment.
Please combine into a single assert statement.
There was a problem hiding this comment.
I'm very surprised to see this check for IsL0 in a test (it was just a performance optimization in the implementation)
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
|
@aarongreig conflicts resolved and UR repo and tag updated :) |
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
@maarquitos14 we resolved the CI issues with #12658, that update also pulls in your changes for this PR so you should just be able to resolve the conflict and we can get this merged |
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
8 participants
Initial implementation to support
sycl_ext_oneapi_composite_devicespecified in #11846.Depends on oneapi-src/unified-runtime#1192.